OCPBUGS-78090: Avoid copy of the oc binary, to support heterogeneous environments#30992
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughReplaces copying the host Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/apiserver/kubeconfigs.go (1)
121-128: Use POSIX whitespace classes for better portability.Lines 122 and 125–128 use
\singreppatterns. While this works in the current GNU grep environment,\sis not part of the POSIX standard for BRE/ERE and may not be reliable across different grep implementations or minimal container environments. Replace with[[:space:]]for guaranteed portability:Proposed fix
- count=$(grep -Ec "^\s*${k}:\s+" "%[1]s") + count=$(grep -Ec "^[[:space:]]*${k}:[[:space:]]+" "%[1]s") [ "$count" -eq 1 ] || { echo "expected exactly one ${k} in %[1]s, got $count" >&2; exit 1; } done -server=$(grep '^\s*server:' "%[1]s" | head -1 | awk '{print $2}') -cert=$(grep '^\s*client-certificate:' "%[1]s" | head -1 | awk '{print $2}') -key=$(grep '^\s*client-key:' "%[1]s" | head -1 | awk '{print $2}') -ca=$(grep '^\s*certificate-authority:' "%[1]s" | head -1 | awk '{print $2}') +server=$(grep -E '^[[:space:]]*server:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}') +cert=$(grep -E '^[[:space:]]*client-certificate:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}') +key=$(grep -E '^[[:space:]]*client-key:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}') +ca=$(grep -E '^[[:space:]]*certificate-authority:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/apiserver/kubeconfigs.go` around lines 121 - 128, The grep patterns use the non-POSIX escape `\s` which can break on some grep implementations; update the patterns in the kubeconfig validation script (the lines that call grep -Ec "^\s*${k}:\s+" and the subsequent grep calls like grep '^\s*server:' , grep '^\s*client-certificate:' , grep '^\s*client-key:' and grep '^\s*certificate-authority:') to use the POSIX whitespace class `[[:space:]]` (e.g. replace occurrences of `^\s*` and `:\s+` with the equivalent `[[:space:]]` forms) so the checks are portable across environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/apiserver/kubeconfigs.go`:
- Line 133: The curl invocation that currently uses the
"${server}/api?timeout=32s" URL (seen in the call that builds the curl command
string in kubeconfigs.go) leaves connection/TLS and network stalls unbounded;
modify the curl command string to add explicit time limits such as
--connect-timeout 10 and --max-time 40 (or similar values) so connection
establishment and the overall request cannot hang indefinitely, ensuring the
constructed command that includes --cert "$cert" --key "$key" --cacert "$ca" and
the "${server}/api?timeout=32s" URL is updated to include these flags.
---
Nitpick comments:
In `@test/extended/apiserver/kubeconfigs.go`:
- Around line 121-128: The grep patterns use the non-POSIX escape `\s` which can
break on some grep implementations; update the patterns in the kubeconfig
validation script (the lines that call grep -Ec "^\s*${k}:\s+" and the
subsequent grep calls like grep '^\s*server:' , grep '^\s*client-certificate:' ,
grep '^\s*client-key:' and grep '^\s*certificate-authority:') to use the POSIX
whitespace class `[[:space:]]` (e.g. replace occurrences of `^\s*` and `:\s+`
with the equivalent `[[:space:]]` forms) so the checks are portable across
environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f545b9aa-b731-4058-b4c0-3f5eeb7dde97
📒 Files selected for processing (1)
test/extended/apiserver/kubeconfigs.go
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-rhcos10-techpreview |
|
@jogeo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/be82cf50-345e-11f1-93cb-693c03651f0f-0 |
|
/retest e2e-aws-ovn-fips |
|
/test e2e-aws-ovn-fips |
1 similar comment
|
/test e2e-aws-ovn-fips |
| eval "val=\$$v" | ||
| [ -n "$val" ] || { echo "${v} field extraction failed from %[1]s" >&2; exit 1; } | ||
| done | ||
| curl -Ssf --connect-timeout 10 --max-time 45 --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output() |
There was a problem hiding this comment.
@codrabbitai could this log any sensitive cert details
|
Thanks for picking this up! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, jogeo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@coderabbitai could this log any sensitive cert details? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6094
Here's why: The kubeconfig fields being extracted are What actually appears in logs:
The curl invocation passes the file paths as One minor note: if a kubeconfig happened to use embedded data fields ( |
|
/override e2e-aws-ovn-fips |
|
@jogeo: jogeo unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@jogeo: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/override ci/prow/e2e-aws-ovn-fips |
|
@sosiouxme: Overrode contexts on behalf of sosiouxme: ci/prow/e2e-aws-ovn-fips DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retitle OCPBUGS-78090: Avoid copy of the oc binary, to support heterogeneous environments |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jogeo: This pull request references Jira Issue OCPBUGS-78090, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jogeo: This pull request references Jira Issue OCPBUGS-78090, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/approved |
|
@jogeo: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@jogeo: Jira Issue Verification Checks: Jira Issue OCPBUGS-78090 Jira Issue OCPBUGS-78090 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
4 participants
Copying the oc binary around fails when the environment is a different architecture or has incompatible versions of glibc.
This change instead uses curl to connect the api endpoint, after extracting the sever and cert details from the kubeconfig.